Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the guidelines github workflow from app-boilerplate #16

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

elric1
Copy link
Contributor

@elric1 elric1 commented Sep 21, 2023

We only had to make minor modifications to get this working. This first to note is that in their guidelines_enforcer.yml, we needed to specify that we build things from a subdirectory. After finding that config option, things are much easier.
They run clang --analyze, so we had to change a few remaining strcpy(3)'s into strlcpy(3).
And, we had to alter the app variant name to tezos_wallet and update the app flags to match what Ledger stores upstream in a DB that they maintain to doublecheck that you aren't changing this without filing a PR against said DB. We preserve the existing flags as specified in that DB, but we believe that the flags should be reviewed as:

  • we are setting the flag to say that this is a library and it isn't [yet], and
  • there are different flags on nanos than nanosp, nanox, and stax.

@elric1 elric1 force-pushed the elric1@upstream-guidelines branch 9 times, most recently from 4c3e1bb to 31b14ad Compare September 21, 2023 20:00
@emturner
Copy link
Collaborator

overall LGTM, but might want to see if we can wait until swap is merged, and then resurrect LedgerHQ/ledger-app-database#69

@elric1 elric1 force-pushed the elric1@upstream-guidelines branch from 31b14ad to e67730d Compare September 24, 2023 17:04
@elric1
Copy link
Contributor Author

elric1 commented Sep 24, 2023

I've rebased this onto #19 because it makes more sense to do that. Let's review #19 first.

@elric1 elric1 force-pushed the elric1@upstream-guidelines branch 3 times, most recently from 2f3d0c9 to 3d0fcff Compare September 27, 2023 20:01
@elric1
Copy link
Contributor Author

elric1 commented Sep 27, 2023

I took 8ed4e83: "In prep for scan-build, we eliminate use of strcpy(3)" out of this and put it in a separate pull request, #28. The PR will not succeed if #28 is not merged first as it solves a failure of one of the checks that these guidelines implement.

@elric1 elric1 force-pushed the elric1@upstream-guidelines branch from 3d0fcff to 6b8d141 Compare September 27, 2023 21:07
@elric1
Copy link
Contributor Author

elric1 commented Sep 27, 2023

Had to rebase this onto #29, because we introduced an issue that broke the CI this introduces we then fixed in #29.

@elric1 elric1 force-pushed the elric1@upstream-guidelines branch from 6b8d141 to 630ec48 Compare September 27, 2023 21:15
app/src/apdu_sign.c Fixed Show fixed Hide fixed
@elric1 elric1 force-pushed the elric1@upstream-guidelines branch 2 times, most recently from e0fde81 to 4e5b15f Compare September 28, 2023 14:57
@elric1 elric1 force-pushed the elric1@upstream-guidelines branch from 4e5b15f to 2ed1c20 Compare September 28, 2023 19:46
@elric1 elric1 merged commit 5505f36 into main Sep 29, 2023
101 checks passed
@elric1 elric1 deleted the elric1@upstream-guidelines branch September 29, 2023 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants